Mounted cloud credentials should not be world-readable#8919
Conversation
kaovilai
left a comment
There was a problem hiding this comment.
Error: pkg/install/daemonset.go:183:1: File is not properly formatted (gofmt)
SecretName: "cloud-credentials",
^
Please fix linters.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8919 +/- ##
==========================================
- Coverage 60.05% 60.02% -0.04%
==========================================
Files 378 378
Lines 42883 42940 +57
==========================================
+ Hits 25754 25775 +21
- Misses 15582 15615 +33
- Partials 1547 1550 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b372610 to
de248e8
Compare
|
The changes LGTM, could you please check the failures in git actions? |
|
|
@kaovilai Hmm. That doesn't seem like it should be related to my change. I guess I'll update the branch (it's out of date anyway) and see what happens on retest. If it's still failig, I'll dig into it in more detail on Monday. |
|
e2e tests are all failing with BSL validation failures. What I don't quite understand is that the only change made by this PR is to update file permissions for the secret to remove the world-read permission. If this caused errors in the e2e Kind cluster, I would expect to see a "permission denied" error originating in velero code, not in the AWS APIs. If we're able to make the AWS call at all, that suggests that velero was able to read the file. If velero was able to read the file, then the AWS API call should be identical with and without this change. |
|
I think it's still related to the credentials permission. |
|
@blackpiglet I need to do some extra logging and see what's going on with the actual velero UID/GID and the file permissions. It could be that the kind clusters don't work properly without everything being world-readable here, or it could be that the e2e tests are doing something weird with the uid/gid for velero causing problems. In theory the velero server should have no problems accessing files with read permission for the owner and group, since the owner and group for the file should be the same as for the velero server process. |
4a68116 to
20b4deb
Compare
|
After adding some logging, it looks like what's happening is that the default credentials, mounted from the secret at |
Thanks for the detailed information. |
|
@sseago As a result, adding the securityContext section to the Velero deployment and the DaemonSet should fix the issue. The Velero server image, Ubuntu Jammy, uses the user I will create a PR to add the SecurityContext for the Velero install CLI and the E2E tests. |
|
@blackpiglet ok, so I figured out what was different between the OpenShift cluster and the Kind e2e cluster. So for mounted secrets, there is no way to change the user ownership. It will always be root/0. However, if the pod sets One possible fix is this:
Another option would be to skip the |
|
@sseago Are there any security hardening rules required to make the secret non-world-readable? If we go the first way, there would be a potential need to modify the fsGroup value when the Velero server image changes. It would be the same if we choose to use a default value. If there is no clear requirement for that, I prefer to go the second way. |
|
@blackpiglet I wasn't suggesting that we hard-code any fsGroup values but making it an optional parameter. If the user doesn't specify fsGroup, then we don't add the security context, and we can just mount it read-only as 444. If the user supplies fsGroup at install, then we set it in the security context and then the velero server will run with that group set. But we could go with the simpler second approach. @weshayutin do you know whether the OADP request for modifying these permissions was related to any specific security hardening rule, or was it just a general sense that we shouldn't have world-readable secrets? I think the practical impact here is relatively small, since in general, users who aren't authorized to access the credentials shouldn't be able to run processes in the velero pod anyway. |
That should work. |
Signed-off-by: Scott Seago <sseago@redhat.com>
|
@blackpiglet Updated with option 2: default creds mounted 444 (read-only), non-default cred files created 640 (no world-read). |
Signed-off-by: Scott Seago <sseago@redhat.com>
Signed-off-by: Scott Seago <sseago@redhat.com>
Signed-off-by: Scott Seago <sseago@redhat.com>
Signed-off-by: Scott Seago <sseago@redhat.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Signed-off-by: Scott Seago <sseago@redhat.com>
* Add BSL status check for backup/restore operations. Signed-off-by: Xun Jiang <xun.jiang@broadcom.com> * Bump golang to v1.23.10 to fix CVEs for 1.16.2 release (velero-io#9058) * Bump golang to v1.23.10 to fix CVEs Signed-off-by: Adarsh Saxena <adarsh.saxena@acquia.com> * Dockerfile restic miss 1.23.10 Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> * restic cve go1.23.10 Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> --------- Signed-off-by: Adarsh Saxena <adarsh.saxena@acquia.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> Co-authored-by: Tiger Kaovilai <tkaovila@redhat.com> * Allow for proper tracking of multiple hooks per container Signed-off-by: Scott Seago <sseago@redhat.com> * Mounted cloud credentials should not be world-readable (velero-io#8919) (velero-io#9094) Signed-off-by: Scott Seago <sseago@redhat.com> * issue 9077: don't block backup deletion on list VS error (velero-io#9101) Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * Fix missing defaultVolumesToFsBackup flag output in Velero describe backup cmd (velero-io#9056) add changelog file Show defaultVolumesToFsBackup in describe only when set by the user minor ut fix minor fix Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> (cherry picked from commit 60a6c73) update changelog filename Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Update Backup describe string for DefaultVolumesToFSBackup flag (velero-io#9105) add changelog file Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> (cherry picked from commit aa2e09c) * Add imagePullSecrets inheritance for VGDP pod and maintenance job. (velero-io#9102) Signed-off-by: Xun Jiang <xun.jiang@broadcom.com> * Bump Golang, Ubuntu, and golang.org/x/oauth2 to fix CVEs. (velero-io#9104) Signed-off-by: Xun Jiang <xun.jiang@broadcom.com> * 1.16.2 changelog Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * Bump the Velero and plugin image versions for the upgrade and migration tests. Signed-off-by: Xun Jiang <xun.jiang@broadcom.com> * skip subresource in resource discovery (velero-io#6688) Signed-off-by: lou <alex1988@outlook.com> Co-authored-by: lou <alex1988@outlook.com> * fix issue 6753 Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * Update restore controller logic for restore deletion (velero-io#6761) 1. Skip deleting the restore files from storage if the backup/BSL is not found 2. Allow deleting the restore files from storage even though the BSL is readonly Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com> * Fix velero-io#6752: add namespace exclude check. Add PSA audit and warn labels. Signed-off-by: Xun Jiang <jxun@vmware.com> * add csi snapshot data movement doc Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * Modify changelogs for v1.12 Signed-off-by: allenxu404 <qix2@vmware.com> * issue 6786:always delete VSC regardless of the deletion policy Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * issue: move plugin depdending podvolume functions to util pkg Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * issue 6880: set ParallelUploadAboveSize as MaxInt64 Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * changelog Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> * Add support for block volumes (velero-io#6680) (velero-io#6897) (cherry picked from commit 8e01d1b) Signed-off-by: David Zaninovic <dzaninovic@catalogicsoftware.com> * Replace the base image with paketobuildpacks image Replace the base image with paketobuildpacks image Fixes velero-io#6851 Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com> * issue 6734: spread backup pod evenly Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * Add doc links for new features to release note Signed-off-by: allenxu404 <qix2@vmware.com> * fix issue 6647 Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * Perf improvements for existing resource restore Use informer cache with dynamic client for Get calls on restore When enabled, also make the Get call before create. Add server and install parameter to allow disabling this feature, but enable by default Signed-off-by: Scott Seago <sseago@redhat.com> * issue velero-io#6807: Retry failed create when using generateName When creating resources with generateName, apimachinery does not guarantee uniqueness when it appends the random suffix to the generateName stub, so if it fails with already exists error, we need to retry. Signed-off-by: Scott Seago <sseago@redhat.com> * Import auth provider plugins Signed-off-by: Sebastian Glab <sglab@catalogicsoftware.com> * Add v1.12.1 changelog Signed-off-by: allenxu404 <qix2@vmware.com> * Make Windows build skip BlockMode code. PVC block mode backup and restore introduced some OS specific system calls. Those calls are not available for Windows, so add both non Windows version and Windows version code, and return error for block mode on the Windows platform. Signed-off-by: Xun Jiang <jxun@vmware.com> * udmrepo use region specified in BSL when s3URL is empty Signed-off-by: Lyndon-Li <lyonghui@vmware.com> * Change v1.12.1 changelog Signed-off-by: allenxu404 <qix2@vmware.com> * Dockerfile.ubi/travis local files add UBI dockerfiles Use numeric user for velero-restic-restore-helper Enable multiarch builds (#135) Use arm64-graviton2 for arm builds (#137) Add required keys for arm builds (#139) Update Travis build job to work w/o changes on new branches Use a full VM for arm Use numeric non-root user for nonroot SCC compatibility * Add BZ + Publish automation to repo (#82) (cherry picked from commit ccb545f) Update PR-BZ automation mapping (#84) (cherry picked from commit aa2b019) Update PR-BZ automation (#92) Co-authored-by: Rayford Johnson <rjohnson@redhat.com> (cherry picked from commit ecc563f) Add publish workflow (#108) (cherry picked from commit f87b779) * remove dependabot config from fork * Create Makefile.prow Code-gen no longer required on verify due to velero-io#6039 Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> oadp-1.2: Update Makefile.prow to velero-restore-helper * set HOME in velero image for kopia, update controller-gen for CI (#280) Signed-off-by: Scott Seago <sseago@redhat.com> * build velero-helper binary for datamover pod * restore: Use warning when Create IsAlreadyExist and Get error Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> * kopia/repository/config/aws.go: Set session.Options profile from config Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> * use ubi9-latest to build * OADP-4225: add tzdata to Dockerfile.ubi * fix: CI (#316) Signed-off-by: Mateus Oliveira <msouzaol@redhat.com> * fix: ARM images (#332) * fix: ARM images Signed-off-by: Mateus Oliveira <msouzaol@redhat.com> * fixup! fix: ARM images Signed-off-by: Mateus Oliveira <msouzaol@redhat.com> --------- Signed-off-by: Mateus Oliveira <msouzaol@redhat.com> * ubi: BUILDPLATFORM to build stage to enable cross compile. (#336) Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> * OADP-4640: Downstream only to allow override kopia default algorithms (#334) (#338) add missing unit test for kopia hashing algo (#337) Introduction of downstream only option to override Kopia default: - hashing algorithm - splitting algorithm - encryption algorithm With introduction of 3 environment variables it is possible to override Kopia algorithms used by Velero: KOPIA_HASHING_ALGORITHM KOPIA_SPLITTER_ALGORITHM KOPIA_ENCRYPTION_ALGORITHM If the env algorithms are not set or they are not within Kopia SupportedAlgorithms, the default algorithm will be used. This behavior is consistent with current behavior without this change. Signed-off-by: Michal Pryc <mpryc@redhat.com> Signed-off-by: Shubham Pampattiwar <shubhampampattiwar7@gmail.com> * Downstream only: Rework of Makefile and incusion of lint The rework of Makefile to make it more readable and inclusion of lint as a target as well extract golangci-lint version from the upstream Dockerfile, so we test in PROW or locally on the same version as upstream. Signed-off-by: Michal Pryc <mpryc@redhat.com> * Downstream only - fix lint error in downtream change (#343) This fixes the PR #334 where one additional line was in the code. This was not exposed previously as we did not had downstream CI Lint jobs. Signed-off-by: Michal Pryc <mpryc@redhat.com> * run oadp-operator e2e test from the velero repo (#353) * run oadp-operator e2e test from the velero repo execute openshift/oadp-operator e2e tests directly against the velero repo locally or via prow ci Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * update variable names, add a cleanup * make sure env variable overrides default velero_image Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * add options to build, push, and only test Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * add arch to name Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * remove duplicated clean/rm operator checkout * simplify by dropping export var and use a oneliner Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> * drop export and use oneliner Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> * just in case, allow oadp to be deployed from makefile Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * Update Makefile.prow Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> --------- Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> * DS Owners * updated controller-gen version * Include velero-restore-helper binary in velero image (#375) Co-authored-by: Scott Seago <sseago@redhat.com> * OADP-5952: downstream only, update error message disableFsBackup (#380) * OADP-5952: clear error for disableFsBackup This error message can be carried in OADP-1.5 Upstream issue: velero-io#8185 Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * fix error message and test --------- Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * Summary of Changes: (#381) Move PVC Request Size Patch to Backup CSI Action Shifted the logic that patches the PVC request size (to match the corresponding VolumeSnapshot size) from the CSI Restore action to the CSI Backup action. ✅ This enables restoring a PVC independently using label selectors, without requiring the VolumeSnapshot to be restored first. Include VolumeSnapshot in CSI Additional Items for PVC Added VolumeSnapshot as an additional item in the PVC CSI backup logic to ensure necessary metadata is available during restore. Include VolumeSnapshotContent in CSI Restore Additional Items Added VolumeSnapshotContent to the additional items in the CSI restore action to support a more complete restore workflow. ✅ This to make sure those resources are restored even if filters out by label from the resources list to restore Author: Amos Mastbaum <amastbau@redhat.com> Signed-off-by: Amos Mastbaum <68001528+amastbau@users.noreply.github.com> fixing-after-michal wait-for-vsc.Status.RestoreSize wait-for-vsc.Status.RestoreSize Update pkg/util/csi/volume_snapshot.go Update pkg/util/csi/volume_snapshot.go Update pkg/util/csi/volume_snapshot.go Co-authored-by: Scott Seago <sseago@redhat.com> * Prep for Konflux (#385) * Prep for Konflux * Update git submodule restic commit --------- Co-authored-by: Rayford Johnson <rayfordj@users.noreply.github.com> * Red Hat Konflux update oadp-velero-oadp-1-5 (#386) * Red Hat Konflux update oadp-velero-oadp-1-5 Signed-off-by: red-hat-konflux <konflux@no-reply.konflux-ci.dev> * hermetic, prefetch-input --------- Co-authored-by: red-hat-konflux <konflux@no-reply.konflux-ci.dev> Co-authored-by: Rayford Johnson <rayfordj@users.noreply.github.com> * Konflux: multiarch, tags, labels (#402) * build-platforms * generate-labels, LABELS * ADDITIONAL_TAGS --------- Co-authored-by: Rayford Johnson <rayfordj@users.noreply.github.com> * Red Hat Konflux update oadp-velero-oadp-1-5 (#411) * Red Hat Konflux update oadp-velero-oadp-1-5 Signed-off-by: red-hat-konflux <konflux@no-reply.konflux-ci.dev> * Konflux: openshift-preflight: failed: HasLicense "suggestion": "Create a directory named /licenses and include all relevant licensing and/or terms and conditions as text file(s) in that directory." https://docs.redhat.com/en/documentation/red_hat_software_certification/2025/html-single/red_hat_openshift_software_certification_policy_guide/index#assembly-requirements-for-container-ima > Container images must contain a “licenses” directory. Use this > directory to add files containing software terms and conditions for your > product and any open source software included in the image. > > Test name: HasLicense --------- Co-authored-by: red-hat-konflux <konflux@no-reply.konflux-ci.dev> Co-authored-by: Rayford Johnson <rayfordj@users.noreply.github.com> * chore(deps): update konflux references (#394) Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com> * chore(deps): update konflux references (#413) Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com> * add velero release to the velero container tags (#424) Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * oadp-1.5: Update Konflux references (#430) * oadp-1.5: Update Konflux references Update konflux-ci image references Changes committed via automation for oadp-1-5/velero. * Use restic's release branch --------- Co-authored-by: Rayford Johnson <rayfordj@users.noreply.github.com> --------- Signed-off-by: Xun Jiang <xun.jiang@broadcom.com> Signed-off-by: Adarsh Saxena <adarsh.saxena@acquia.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> Signed-off-by: Scott Seago <sseago@redhat.com> Signed-off-by: Lyndon-Li <lyonghui@vmware.com> Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> Signed-off-by: lou <alex1988@outlook.com> Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com> Signed-off-by: Xun Jiang <jxun@vmware.com> Signed-off-by: allenxu404 <qix2@vmware.com> Signed-off-by: David Zaninovic <dzaninovic@catalogicsoftware.com> Signed-off-by: Sebastian Glab <sglab@catalogicsoftware.com> Signed-off-by: Mateus Oliveira <msouzaol@redhat.com> Signed-off-by: Michal Pryc <mpryc@redhat.com> Signed-off-by: Shubham Pampattiwar <shubhampampattiwar7@gmail.com> Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com> Co-authored-by: Xun Jiang <xun.jiang@broadcom.com> Co-authored-by: Wenkai Yin(尹文开) <yinw@vmware.com> Co-authored-by: Adarsh Saxena <adarsh.saxena@acquia.com> Co-authored-by: Tiger Kaovilai <tkaovila@redhat.com> Co-authored-by: Scott Seago <sseago@redhat.com> Co-authored-by: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com> Co-authored-by: Shubham Pampattiwar <spampatt@redhat.com> Co-authored-by: Xun Jiang/Bruce Jiang <59276555+blackpiglet@users.noreply.github.com> Co-authored-by: Lyndon-Li <lyonghui@vmware.com> Co-authored-by: Daniel Jiang <jiangd@vmware.com> Co-authored-by: lou <alex1988@outlook.com> Co-authored-by: Xun Jiang <jxun@vmware.com> Co-authored-by: allenxu404 <qix2@vmware.com> Co-authored-by: David Zaninovic <74072514+dzaninovic@users.noreply.github.com> Co-authored-by: Sebastian Glab <sglab@catalogicsoftware.com> Co-authored-by: Dylan Murray <dymurray@redhat.com> Co-authored-by: RayfordJ <rayfordj@users.noreply.github.com> Co-authored-by: Mateus Oliveira <msouzaol@redhat.com> Co-authored-by: Wesley Hayutin <138787+weshayutin@users.noreply.github.com> Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> Co-authored-by: OpenShift Cherrypick Robot <openshift-cherrypick-robot@redhat.com> Co-authored-by: RayfordJ <4580787+rayfordj@users.noreply.github.com> Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux <konflux@no-reply.konflux-ci.dev>
Thank you for contributing to Velero!
Please add a summary of your change
Cloud credentials (both default and BSL-named) should not be world-readable. This PR makes the file access permissions more restrictive.
Does your change fix a particular issue?
Fixes #8906
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main.